Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toast for Service Worker #2426

Closed
wants to merge 7 commits into from
Closed

Conversation

viankakrisna
Copy link
Contributor

Added Toast component for Service Worker, inspired by #2398
It only listens for first registration and update events. I tried to add one to indicate that the content is served from service worker, but it's annoying because it's always shown on every refresh.

may-31-2017 23-01-41

Copy link

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this started. I'll defer to @gaearon as he had some opinions about the effectiveness of toasts from the other thread we're discussing solutions on.

}

Toast.defaultProps = {
timeout: 3000,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 3s timeout is consistent with some of the other implementations that have opted for this pattern in case anyone is wondering: https://github.com/GoogleChrome/voice-memos/blob/bcb225a41d3dfcec5e9805adf33d27995840c9f0/src/scripts/libs/Toaster.js#L41.

Copy link
Contributor Author

@viankakrisna viankakrisna May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also seen "dismiss" button some implementation, but I think we can leave it to the users? edit: nvm, updated it with dismiss button

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think you made the right call with the latest set of updates around dismissal.

@@ -39,6 +60,8 @@ export default function register() {
console.error('Error during service worker registration:', error);
});
});
} else {
renderToast('Development mode started.');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the goal of this to ease users into the idea of the toast UI now being available? I haven't seen other implementations of the toast showing a dev mode message but perhaps it's another awareness moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so they can see how it look if they want to style it differently :)

} else {
// At this point, everything has been precached.
// It's the perfect time to display a
// "Content is cached for offline use." message.
console.log('Content is cached for offline use.');
showMessage('Content is cached for offline use.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible addition would be to also include a "learn more" link that pointed to the documentation for additional context. Using a timeout of longer than 3 seconds might be appropriate in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this code is running on production. I don't think it's a good default for the user to have a link to create-react-app in their production code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffposnick I added readme link in dev mode but I think it would be made more sense to link it to the full readme.md instead of the specific section of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Service Workers are not enabled in development mode, maybe we should let the developer know why the toast exists.

We could also add some information in registerServiceWorker.js about how to remove service workers.

"In production your users will see this toast to notify them service workers are enabled. See registerServiceWorker.js for more info"

What about instead of using the word 'content' we say 'app'.

A new version of this app is available. Please refresh your browser.

This app had been made available for offline use.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a nice enhancement if you make the word refresh as a link on click of which a refresh is triggered?

@viankakrisna
Copy link
Contributor Author

Updated the style to take up entire window width and add dismiss button.
may-31-2017 23-45-03

@@ -0,0 +1,39 @@
.Toast {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth name spacing this, if there is any expectation the toast will appear in production?

.cra-Toast maybe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to use this as a template for the user to customize it. So I think it's okay to leave it not namespaced. CMIIW

@piotr-cz
Copy link
Contributor

piotr-cz commented Jun 8, 2017

I miss an option to switch notification and bundled stylesheets off by config option.
Such config option could be shown in the toast

}

.Toast a,
.Toast-button {
Copy link
Contributor

@ro-savage ro-savage Jun 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this behave more like a link?

cursor: pointer

Toast-button:hover { text-decoration: underline }

And maybe remove the uppercasing from all links, and change it to just so DISMISS is in uppercase?

line-height: 24px;
border-top-left-radius: 2px;
border-top-right-radius: 2px;
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a z-index just to ensure this is on top?

const domId = 'toast';
let dom = document.getElementById(domId);

if (!dom) {
Copy link
Contributor

@ro-savage ro-savage Jun 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is suppose to be once off on load. Maybe we should remove the created dom node after the timeout ends / it's dismissed.

Don't think its necessary but could be nice to avoid any possibility of conflicting styles / layout.

@ro-savage
Copy link
Contributor

This looks great @viankakrisna

I've tested a bit across browsers + sizes (no IE though) and worked fine.

I have made a couple optional suggestions. I'd like to see this merged in asap.

@viankakrisna
Copy link
Contributor Author

@ro-savage thanks for the feedback! I'll hold off making changes until we are sure where we are going with service worker. On a second thought, maybe inline style is the way to go for this toast for the issue with the namespace. If SW ends up being an optional feature, I don't think we need the Toast component to be in a separate file (too much noise for the user).

@react-scripts-dangerous

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit 873a617) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.15-873a617.0
# or
yarn add react-scripts-dangerous@1.0.15-873a617.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.15-873a617.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

@@ -56,12 +95,12 @@ function registerValidSW(swUrl) {
// the fresh content will have been added to the cache.
// It's the perfect time to display a "New content is
// available; please refresh." message in your web app.
console.log('New content is available; please refresh.');
showMessage('New content is available; please refresh.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible UX improvements:

  1. add link which user can click to reload page:
New content is available, <a onclick="window.location.reload()" >please refresh</a>.
  1. Do not show message about reload and instead reload browser when user navigates to the next page. But this makes sense only if app actually has routes and uses something like react-router

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice idea! I am waiting for #2755 to get in so we can solve #2638 and continue working on SW enhancements

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2755 closed BTW

@piotr-cz
Copy link
Contributor

As this PR got somehow stuck, I've decided to prepare an other one, dealing just with callbacks inside registerServiceWorker file: #3375

I'm using this technique for few months and I'm happy with it.
Changes in code base are minimal

},
this.props.timeout
);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better adding space between functions?

@Timer
Copy link
Contributor

Timer commented Sep 26, 2018

Closing as part of #4169; I'd love to see this released as a user-land feature though we can add to the documentation.

Thanks for all your efforts!

@Timer Timer closed this Sep 26, 2018
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.